Conversation
…bfolder resolve_git_common_dir() only stats repo_path/.git directly, so indexing a subfolder with no .git of its own never consulted any ancestor repo's .gitignore. Add resolve_enclosing_git_root() to walk up parent directories the way git itself does, and merge that ancestor's .gitignore (and info/exclude) ahead of the subfolder's own so the more specific file still wins on conflict. Fixes DeusData#510 Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
The one red leg here ( |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you for this, and apologies that it sat eleven days without a maintainer word — that is on us, not you. The diagnosis is right (#510's second half is exactly "the enclosing repo's rules are never consulted from a git-less subfolder"), the parent walk is sound and bounded (it stops at /, at a bare drive root and at a separator-less string; I traced UNC and //?/ spellings and they only add a stat probe), the symmetry with resolve_git_common_dir is the right shape, and the three tests are RED for the right reason: with the production hunk reverted on today's main (8b57fe2a) exactly discover_enclosing_repo_gitignore_issue510, discover_enclosing_repo_info_exclude and discover_enclosing_repo_gitignore_local_overrides fail (count == 2, expected 1), and re-applied it is 124/124 on discover, 309/309 on gitignore + pipeline, memory-core linter unchanged, merge with main clean.
One thing blocks it, and it is a correctness problem rather than polish: anchored patterns from the enclosing repo are re-anchored to the indexed subfolder. The ancestor's patterns are merged into the same matcher whose rel_path is relative to repo_path (src/discover/discover.c, the cbm_gitignore_merge after resolve_enclosing_git_root), and a rooted pattern is matched against that path directly (src/discover/gitignore.c, p->rooted ? glob_match_bounded(p->pattern, rel_path) : …). Git evaluates every .gitignore relative to the directory that contains it. Two probes on the merge result, each checked against git check-ignore run from pkg/:
- root
.gitignore=/secret.py→ this branch skipspkg/secret.py; git does not ignore it. A root line like/build,/distor/src(very common) now hides the same-named entry inside every indexed subfolder — silent index loss, which is the one thing discovery must never do. - root
info/exclude=pkg/scratch/→ this branch indexespkg/scratch/tmp.py; git ignores it. So the "secrets and build output get indexed" case from #510 persists for exactly the anchored form people write for a subdirectory.
Your three tests all use unanchored patterns (secret.py, *.log, scratch/), which match at any depth, so the suite cannot see either direction.
What I would ask for:
- Keep the enclosing repo's matcher(s) separate instead of merging them into
repo_path's, and evaluate each against the path relative to the directory its file lives in — i.e.<repo_path relative to that directory>/<rel_path>. Thegitignore_link_tchain indiscover.calready encodes "deepest file with an opinion wins" for directories below the walk root; this is the mirror image (a base offset applied torel_path) rather than a plain merge. - Load the
.gitignoreof every directory from the enclosing root down torepo_path, shallow to deep, not only the root's — git consults all of them. - Canonicalize
repo_path(cbm_canonical_pathinsrc/foundation/compat_fs.h) before the lexical walk: as written,cbm index pkgandcbm index /abs/path/pkgdiscover different things, because the walk is purely lexical on the caller's string. - Tests for both anchored directions, mirroring the two probes: root
/secret.pymust not hidepkg/secret.py, rootpkg/scratch/must hidepkg/scratch/tmp.py, plus one intermediate-directory.gitignorecase. - Small and optional: on a failed merge the code currently drops the more specific local patterns and keeps the ancestor's (the inverse of the documented policy on the
info/excludemerge just below), and settingis_git_repo = truenewly enables the user's globalcore.excludesfilefor git-less subfolders — correct per git, but worth a line in the PR body since it is a visible behaviour change.
The Windows guard failure on your run is ours (a cold-start race in the daemon endpoint, fixed in #2275), not anything in this diff; I will update the branch once that lands so you get a clean run. If you would rather I take the base-relative matching from here, say so and I will, with your commits kept as the foundation — but the design is yours and I would be glad to see you finish it.
|
You're right, and the two probes make it obvious once I see them: root Take it from here. The matcher-offset redesign and the chain walk down from the enclosing root need the kind of familiarity with Thanks for tracing the Windows failure to #2275, that saves me chasing a dead end. |
…to their own directory
The previous commit found the enclosing repository when a git-less subfolder
is indexed, but merged its patterns into the matcher whose rel_path is
relative to repo_path. gitignore.c matches a rooted pattern with
`glob_match_bounded(p->pattern, rel_path)`, so every anchored pattern of the
enclosing repository was silently re-anchored onto the indexed subfolder.
That is wrong in both directions, and both were checked against real
`git check-ignore` run inside the subfolder:
* enclosing root .gitignore `/secret.py`, indexing pkg/
git INDEXES pkg/secret.py — we HID it. A rooted pattern means "in the
repository root", and re-anchored it became "in pkg". Silent index loss:
the file is gone from the graph with no diagnostic, the worst failure
mode discovery has.
* enclosing .git/info/exclude `pkg/scratch/`, indexing pkg/
git IGNORES pkg/scratch/tmp.py — we INDEXED it. The pattern is anchored
THROUGH pkg, so once rel_path lost the "pkg/" prefix it stopped matching
and an excluded directory got walked.
Only the enclosing root's .gitignore was consulted, too; git reads the
.gitignore of every directory between the repository root and the indexed
one, deeper overriding shallower.
Mechanism. discover.c already had the chain that encodes "the deepest file
with an opinion wins": gitignore_link_t carries a `prefix`, the walk-relative
directory a nested matcher came from, which local_rel_path() strips off. An
ancestor is the mirror image of that, so the chain is extended rather than
duplicated: each link now also carries a `base`, the walk root's path
relative to the directory the matcher came from, which gitignore_chain_result
PREPENDS instead of stripping (discover.c:569, 610). Exactly one of the two
offsets is ever non-empty, and both live in the link's flexible tail, so no
allocation site is added.
* ancestor_ignores_build() (discover.c:1282) loads the .gitignore of every
directory from the enclosing repository root down to — but not including —
the indexed one, shallow to deep, and chains them so the deepest wins,
negations included. The enclosing repository's info/exclude is merged into
the ROOT ancestor's matcher, which is where git anchors it, keeping the
precedence info/exclude already has over .gitignore at the same level.
* the indexed directory's own .gitignore stays its own matcher at the leaf
of that chain (discover.c:1480), so it still overrides every ancestor.
Nothing is merged into it any more, which also removes the earlier merge's
failure mode of dropping the local patterns and keeping the ancestor's.
* repo_path is canonicalized with cbm_canonical_path() before the lexical
parent walk (discover.c:1414), so `cbm index pkg` and
`cbm index /abs/path/pkg` discover the same files; pipeline.c strdups the
caller's path unchanged.
* walk_owned_gitignore_free() (discover.c:1019) is factored out of walk_dir
and shared with the ancestor chain, which owns its matchers the same way.
Proof on real input, before the suites. Indexing scripts/ of this checkout —
a git-less subfolder whose root .gitignore carries the rooted patterns
`/memlab-*` and `/soak*/`:
before 87 files memlab-drive.py MISSING memlab-report.py MISSING
after 89 files memlab-drive.py present memlab-report.py present
`git check-ignore` inside scripts/ indexes both, so the two recovered files
are exactly the ones git keeps. No file is lost in the other direction.
RED on the previous commit, four new tests, each direction pinned to a
`git check-ignore` verdict on an identical fixture:
test_discover.c:1471: count == 1, expected 2 == 2 rooted_pattern_not_reanchored
test_discover.c:1498: count == 2, expected 1 == 1 info_exclude_rooted_subpath
test_discover.c:1526: count == 2, expected 1 == 1 intermediate_gitignore
test_discover.c:1555: count == 0, expected 1 == 1 deeper_ancestor_negation_wins
discover: 120 passed, 4 failed
The three tests already on the branch use unanchored patterns, which match at
any depth and so cannot see this bug; they pass before and after.
GREEN: discover 124 passed, 0 failed; discover + gitignore + pipeline +
git_context 425 passed, 0 failed.
Revert-check: reverting only the discover.c hunks and rebuilding puts the
four new tests RED again on the same four lines with the same counts
(120 passed, 4 failed); restoring returns 425 passed. Memory-core linter:
none grew (31 raw sites in discover.c before and after). clang-format clean.
Co-authored-by: Amir Fathi <amirfathi.me@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for handing this over so clearly, and for keeping the door open instead of guessing at the chain code. I took you at your word: your commit stays exactly as you wrote it, as the foundation, and I added one commit on top of it on this branch, with you credited as co-author. What the added commit does:
Tests: four new cases, each pinned first to real The real-input check is the part I like best: this repository's own Honest limits: this was run on macOS only; the Windows legs of CI on this push are the first Windows run of the change, and the drive-root and UNC stop conditions are still your original loop, untouched. Symlinked |
This branch has not been deployed
2 participants
discover_implonly ever loaded.gitignorefromrepo_pathitself (fixed for the first half of #510 by 3f754cf) and, whenrepo_pathcarries its own.git, from that repo'sinfo/exclude. Whenrepo_pathis a subfolder with no.gitof its own, the enclosing repository's root.gitignorewas never consulted, so files the enclosing repo ignores (secrets, build output) get indexed and become searchable.Added
resolve_enclosing_git_root(): whenresolve_git_common_dir(repo_path, ...)fails, walk up parent directories the same waygitdoes from a subfolder, and load the first ancestor's.gitignore(plus itsinfo/exclude, sinceis_git_repobecomes true once an ancestor is found). It's merged ahead ofrepo_path's own.gitignoreso the more specific file still wins on conflict viacbm_gitignore_merge, matching git's shallow-to-deep precedence.Added three tests: enclosing root
.gitignorehonored, enclosinginfo/excludehonored, and the indexed subfolder's own.gitignorestill overriding the enclosing one on conflict. All three go red without the fix. Ranscripts/test.sh(every suite, ASan+UBSan) on this branch and it's fully green.clang-formatis clean on both files;cppcheck/clang-tidyflag a couple of pre-existing things incompat_regex.c/cli.c/mcp.cthat this diff doesn't touch, same result on plain main.Fixes #510